Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More robust verification method selection by did #3279

Merged
merged 9 commits into from
Nov 12, 2024

Conversation

dbluhm
Copy link
Contributor

@dbluhm dbluhm commented Oct 9, 2024

This PR is intended to supersede #3138 by adding a default verification method selection strategy for all DID methods.

This strategy relies on proof type and proof purpose to select the most appropriate verification method. For example, if proof type is Ed25519Signature2020 and purpose is assertionMethod, the changes in this PR will select the first matching method in the assertionMethod relationship that can produce a Ed25519Signature2020.

This places more expectations on the DID -- specifically that it is well formatted (which many aren't). By well formatted I mean that verification methods used for issuance are properly referenced in assertionMethod, methods used to authenticate the DID owner are in authentication, etc. In practice, this may cause challenges but it is possible for users who depend on a DID method/document that isn't well formatted to create their own strategy and plug it in. So I'm comfortable making the assertion that we can expect the DIDs we're working with to be well formatted.

This PR does adjust the interface for the VerificationKeyStrategy to better account for this. This might be painful to some plugin users who've added DID Methods by plugin and added a strategy to match. However, my hope is that those same users won't need to plug in their own strategy anymore with these changes since they should be robust enough to cover most use cases.

Shout out to @aritroCoder for his original work on #3138!

@dbluhm dbluhm requested a review from jamshale October 9, 2024 20:03
@dbluhm
Copy link
Contributor Author

dbluhm commented Oct 9, 2024

@PatStLouis please reivew (I couldn't formally request your review for some reason)

@dbluhm
Copy link
Contributor Author

dbluhm commented Oct 9, 2024

Because of the changes to the selection strategy from 3138, it's possible I haven't fully accounted for the original issue yet. I will take a closer look. I suspect we just need the prover side of the exchange to use the correct proof purpose when determining vm id.

Edit: I think I've address this now with the most recent commit.

@dbluhm
Copy link
Contributor Author

dbluhm commented Oct 9, 2024

@jamshale your input on BDD interop test failure would be appreciated. Is this an OWF migration artifact or something else?

@jamshale
Copy link
Contributor

jamshale commented Oct 9, 2024

@jamshale your input on BDD interop test failure would be appreciated. Is this an OWF migration artifact or something else?

Ya. I changed a reference I shouldn't have. Should have a fix ready soon.

Copy link
Contributor

@PatStLouis PatStLouis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. A few assumptions are being made about proof purposes but I think the decisions here makes sense.

@PatStLouis
Copy link
Contributor

@dbluhm thanks for doing this work. @aritroCoder does this cover your use case?

Copy link

sonarcloud bot commented Oct 10, 2024

Copy link
Contributor

@jamshale jamshale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me but I'm not very familiar with the use case or consequences. The approach seems to make sense.

@dbluhm
Copy link
Contributor Author

dbluhm commented Oct 10, 2024

I need to fix unit tests. Changes should be captured and tested at least in the json-ld scenario integration test but might be interesting/a good idea to have a more directed test of the functionality.

@swcurran swcurran marked this pull request as draft October 11, 2024 18:44
@swcurran
Copy link
Contributor

Want to defer a merge until we get 1.1.0 release out.

@swcurran swcurran marked this pull request as ready for review October 22, 2024 16:12
@dbluhm
Copy link
Contributor Author

dbluhm commented Nov 11, 2024

Unit tests fixed; this PR is ready for final review and merge

Copy link
Contributor

@PatStLouis PatStLouis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

)

if len(methods) > 1:
LOGGER.info(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great

aritroCoder and others added 9 commits November 12, 2024 10:42
Signed-off-by: aritroCoder <[email protected]>
added multikey support

Co-authored-by: Patrick St-Louis <[email protected]>
Signed-off-by: Aritra Bhaduri <[email protected]>
added multikey support

Co-authored-by: Patrick St-Louis <[email protected]>
Signed-off-by: Aritra Bhaduri <[email protected]>
added multikey support

Co-authored-by: Patrick St-Louis <[email protected]>
Signed-off-by: Aritra Bhaduri <[email protected]>
Copy link

sonarcloud bot commented Nov 12, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
63.9% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@dbluhm dbluhm merged commit b7d27fa into openwallet-foundation:main Nov 12, 2024
9 of 10 checks passed
@dbluhm dbluhm deleted the feature/vm-selection branch November 12, 2024 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants